[WIP] Emit logs for authentication failures and successes (local and oidc authenticator plugins #726
Conversation
markafarrell
force-pushed
the
feature/log-authentication-attempts
branch
from
b3d8fd0 to
243713d
Compare
markafarrell
force-pushed
the
feature/log-authentication-attempts
branch
from
243713d to
737b931
Compare
|
DVCS PR Check Results: Could not find JIRA key(s) in PR title, branch name, or commit messages |
@BrennanPaciorek Changed to |
| f"REMOTE_HOST: {request.META['REMOTE_HOST'] if 'REMOTE_HOST' in request.META else 'UNKNOWN'}" | ||
| ) | ||
|
|
||
| logger.info(f"Login attempt for user: {username} {auth_log_headers}") |
There was a problem hiding this comment.
Should we make this a debug log level or do we feel like these headers are info level worthy?
| logger.info(f"Successful login for user: {username} {auth_log_headers}") | ||
| else: | ||
| logger.info(f"Failed login for user: {username} {auth_log_headers}") |
There was a problem hiding this comment.
We are logging the auth headers here again, I think we can drop it out of these messages.
There was a problem hiding this comment.
I think its valuable to have the status and the headers in a single log entry. It lets you easily determine where the successes/failures are coming from. I think we can probably drop the attempt log to debug
| def authenticate(self, *args, **kwargs): | ||
| request = args[0] | ||
|
|
||
| auth_log_headers = ( | ||
| f"HTTP_USER_AGENT: {request.META['HTTP_USER_AGENT'] if 'HTTP_USER_AGENT' in request.META else 'UNKNOWN'} " | ||
| f"HTTP_X_FORWARDED_FOR: {request.META['HTTP_X_FORWARDED_FOR'] if 'HTTP_X_FORWARDED_FOR' in request.META else 'UNKNOWN'} " | ||
| f"REMOTE_ADDR: {request.META['REMOTE_ADDR'] if 'REMOTE_ADDR' in request.META else 'UNKNOWN'} " | ||
| f"REMOTE_HOST: {request.META['REMOTE_HOST'] if 'REMOTE_HOST' in request.META else 'UNKNOWN'}" | ||
| ) | ||
|
|
||
| if "backend" in kwargs and kwargs["backend"].name == self.name: | ||
| logger.info(f"Login attempt for {auth_log_headers}") | ||
|
|
||
| user = super().authenticate(*args, **kwargs) | ||
|
|
||
| if "backend" in kwargs and kwargs["backend"].name == self.name: | ||
| if user: | ||
| logger.info(f"Successful login for {user} {auth_log_headers}") | ||
| else: | ||
| logger.info(f"Failed login {auth_log_headers}") | ||
|
|
||
| return user | ||
|
|
There was a problem hiding this comment.
Should we move this to the SocialAuthMixin so that all of the classes derived from that get this logging? Same comments about logging the auth_log_headers as debug and removing it from the "Successful/Failed" login messages.
There was a problem hiding this comment.
Probably a good idea.
john-westcott-iv
changed the title
Description
Modifes the local and oidc authentication plugins so that logs are emitted for:
This assist with audit user accesses
Logs are emitted that can be inspected to audit what users are logging in and if any failed login attempts are occurring
Type of Change
Self-Review Checklist
Testing Instructions
Login using either local or oidc and check logs for emitted logs
Prerequisites
N/A
Steps to Test
Expected Results
Failed local login
Successful oidc login
Successful local login
Additional Context
Required Actions
Screenshots/Logs